[v3.0.0] version callbacks #358

zfields · 2017-03-12T21:16:47Z

Here is the motivation behind the pull-request, as fleshed out below...

Originally, the parser was built into FirmataClass which serves the host/server.
There was no notion of a callback for version, ever; it was tightly coupled.
REPORT_VERSION was used to query the version from the host/server, and it would subsequently report the version using REPORT_VERSION.
The fact that REPORT_VERSION is a single byte operation in the protocol documentation is wrong, because the version is actually reported using three bytes.
Therefore, REPORT_VERSION should always send three bytes and the server/host should be smart enough realize it is being queried and discard them.
An alternate approach would be to create a single byte QUERY_VERSION used to explicitly query the version from the server/host

should follow a similar approach to REPORT_FIRMWARE

zfields · 2017-03-12T21:28:55Z

This causes some conflict with the node sample on the ESP8266. I don't know node all that well, so I'm not confident that I can debug it.

Does firmata-js use FirmataParser? I changed it's callback signature to do this, but I didn't change the FirmataClass API, so I'm not sure why this is messing things up...

soundanalogous · 2017-03-13T00:44:44Z

firmata.js doesn't use FirmataParser at all. It simply parses based on the way Firmata.cpp used to do things.

soundanalogous · 2017-03-13T01:01:04Z

I have to admit I'm starting to get lost in all of these callbacks.

Does this codepath still run in some form? https://github.com/firmata/arduino/blob/2.5.4/Firmata.cpp#L240.

And this?
https://github.com/firmata/arduino/blob/2.5.4/Firmata.cpp#L369

soundanalogous · 2017-03-13T02:00:16Z

FirmataMarshaller.cpp

@@ -168,6 +168,8 @@ const
  if ( (Stream *)NULL == FirmataStream ) { return; }
  FirmataStream->write(START_SYSEX);
  FirmataStream->write(REPORT_FIRMWARE);
+  FirmataStream->write(firmata::FIRMWARE_MAJOR_VERSION);


This is a query for the firmware version so you don't know the version at this point.

Maybe what is needed here instead is two new Firmata commands QUERY_FIRMWARE_VERSION and QUERY_PROTOCOL_VERSION and those have no payload. Only the Reply has data.

I totally agree. Currently, these additional bytes would be discarded by FirmataClass, however they cause the callbacks to be invoked correctly.

I still don't understand. These bytes aren't specified in the protocol for the query.

I guess it doesn't matter since the firmware doesn't even need this. Only c++ client libraries.

Yes, that's right. Currently, FirmataClass responds to this query, by putting the byte back on the wire and adding the version bytes behind it (which breaks the protocol). However, if someone would like to get the version bytes via the parser (theoretically, this would be the primary way), then we need to change the parser to be able to consume the additional bytes that provide the version information and change the protocol to provide them (which it currently does not).

As you pointed out, this could be accomplished by creating QUERY_FIRMWARE_VERSION and QUERY_PROTOCOL_VERSION, then migrating logic for REPORT_FIRMWARE and REPORT_VERSION (whose functionality ironically queries) over to them and keep the changes I've proposed to handle the actual reporting of versions and not the query.

soundanalogous · 2017-03-13T02:00:23Z

FirmataMarshaller.cpp

@@ -179,6 +181,8 @@ const
 {
  if ( (Stream *)NULL == FirmataStream ) { return; }
  FirmataStream->write(REPORT_VERSION);
+  FirmataStream->write(firmata::PROTOCOL_MAJOR_VERSION);


Yup, just fluff.

zfields · 2017-03-13T04:33:31Z

To answer your previous question. Yes those code paths get executed, but in the static callbacks of FirmataClass.h.

https://github.com/firmata/arduino/blob/master/Firmata.h#L153
https://github.com/firmata/arduino/blob/master/Firmata.h#L154

soundanalogous · 2017-03-13T05:06:33Z

FirmataParser.cpp

@@ -130,6 +130,9 @@ void FirmataParser::parse(uint8_t inputData)
          if (currentReportDigitalCallback)
            (*currentReportDigitalCallback)(currentReportDigitalCallbackContext, multiByteChannel, dataBuffer[0]);
          break;
+        case REPORT_VERSION:
+          if (currentReportVersionCallback)
+            (*currentReportVersionCallback)(currentReportVersionCallbackContext, dataBuffer[0], dataBuffer[1], (const char *)NULL);


Firmata client libraries do not provide data parameters for REPORT_VERSION so I'm not sure where the version data would come from. You could pass it in statically though. This is probably why the ESP8266 isn't working.

I am aware, but I need to standardize the message so the parser can parse it - even if portions are disregarded. The name of the byte is REPORT version, not QUERY. You happen to be using it for both.

I think I'm starting to understand. Let me know if this is correct.

The version parameters are needed in the callback since the callback is actually for the reply rather than the query. When the callback is called, those parameters are available.

The confusion on the protocol side is that REPORT_VERSION and REPORT_FIRMWARE were shared for both query and reply (which was common in the early days of Firmata).

I see since we can anticipate that dataBuffer[0] and dataBuffer[1] will contain the appropriate data when the callback is called. I was overlooking the fact that dataBuffer was a member variable, that's what was largely throwing me off. Maybe we should have a convention member variables. I've been prefixing with m for the SPI feature I've been working on: https://github.com/firmata/arduino/blob/spi-alpha/utility/SPIFirmata.h#L94-L96.

Originally, the parser was built into FirmataClass which serves the host/server.

There was no notion of a callback for version, ever; it was tightly coupled.

REPORT_VERSION was used to query the version from the host/server, and it would subsequently report the version using REPORT_VERSION.

The fact that REPORT_VERSION is a single byte operation in the protocol documentation is wrong, because the version is actually reported using three bytes.

Therefore, REPORT_VERSION should always send three bytes and the server/host should be smart enough realize it is being queried and discard them.

An alternate approach would be to create a single byte QUERY_VERSION used to explicitly query the version from the server/host

REPORT_FIRMWARE should follow a similar approach

I'll add QUERY_FIRMWARE_VERSION and QUERY_PROTOCOL_VERSION for v3.0 since I can't really think of a good reason for a client library to send it's version to the board.

I prefix all my _member variables on my projects, so I'm down. I was just following style.

soundanalogous · 2017-03-13T05:08:52Z

FirmataParser.cpp

-      if (currentReportFirmwareCallback)
-        (*currentReportFirmwareCallback)(currentReportFirmwareCallbackContext);
+      if (currentReportFirmwareCallback) {
+        size_t sv_major = dataBuffer[1], sv_minor = dataBuffer[2];


same here. This data would never be in the buffer with existing Firmata client libraries.

Couldn't you just use the constants: https://github.com/firmata/arduino/blob/master/FirmataConstants.h#L22-L23?

No. I would have compiled in the constants, they describe the version on my side of the protocol not the other side.

soundanalogous · 2017-03-13T05:48:33Z

FirmataParser.cpp

@@ -163,8 +166,8 @@ void FirmataParser::parse(uint8_t inputData)
        systemReset();
        break;
      case REPORT_VERSION:
-        if (currentReportVersionCallback)
-          (*currentReportVersionCallback)(currentReportVersionCallbackContext);
+        waitForData = 2; // two data bytes needed


This is where I wish we had unit test coverage for the parser. I'm still not 100% sure this will not break with most existing Firmata client libraries.

I'd want to know that if waitForData is 2 or 0 this will still work as expected.

Yeah tracing through the code, this will cause the next two bytes after a REPORT_VERSION command is sent (as a query) from a client library to be skipped. So if REPORT_VERSION is followed immediately by a Sysex message (which I believe is the case in firmata.js) then that entire Sysex message will be lost (since the START_SYSEX and COMMAND bytes will be skipped).

Actually START_SYSEX will get through because ( (waitForData > 0) && (inputData < 128) )
But I don't think the COMMAND byte will because waitForData would still equal 2. One work around is in the START_SYSEX case, reset waitForData to 0.

It does change the behavior... With this change, you will need to send all three bytes for the version in order to get the callback from FirmataClass to fire. However, it is worth noting that the unsolicited sketch startup REPORT_VERSION message fires, regardless.

Conceivably REPORT_FIRMWARE should work the same either way. On the host side, would just pull some junk out of the buffer for the version info, but it would turn around and discard it immediately.

Perhaps it would be good to split these up. It would allow us to pull in REPORT_FIRMWARE and leave REPORT_VERSION outstanding for v3.0.0

That's going to break a lot of client libraries. We'll have to wait for Firmata v3.0 then.

This is because many Firmata client libraries initiate communication with the board by querying the protocol version.

I think there is some general confusion here because REPORT_VERSION is misleading. Basically a client should never report its version to the host/firmware. A client can only query the firmware and protocol versions from the host/firmware.

But you're trying to use the same parser both on the host and client side so now I understand the larger issue here.

zfields · 2017-03-15T02:41:41Z

I have rebased this branch on top of the recent FIRMWARE_VERSION update. Thus, resolving any obvious, or known, merge conflicts and making it more "cherry-pick"-able when v3.0.0 comes around.

zfields force-pushed the version branch 2 times, most recently from 94aba8f to b749d3f Compare March 12, 2017 22:14

soundanalogous reviewed Mar 13, 2017

View reviewed changes

zfields changed the title ~~version callbacks~~ [v3.0.0] version callbacks Mar 13, 2017

zfields force-pushed the version branch 2 times, most recently from de43cb2 to 939e332 Compare March 15, 2017 02:03

version callbacks

fbe73f2

zfields force-pushed the version branch from 939e332 to fbe73f2 Compare March 15, 2017 02:12

soundanalogous added this to the 3.0 milestone Mar 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v3.0.0] version callbacks #358

[v3.0.0] version callbacks #358

zfields commented Mar 12, 2017 •

edited

Loading

zfields commented Mar 12, 2017 •

edited

Loading

soundanalogous commented Mar 13, 2017

soundanalogous commented Mar 13, 2017

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

zfields Mar 13, 2017

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017

zfields Mar 13, 2017

zfields commented Mar 13, 2017

soundanalogous Mar 13, 2017 •

edited

Loading

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017 •

edited

Loading

zfields Mar 13, 2017

soundanalogous Mar 13, 2017

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

zfields Mar 13, 2017

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017

soundanalogous Mar 13, 2017

zfields commented Mar 15, 2017

[v3.0.0] version callbacks #358

Are you sure you want to change the base?

[v3.0.0] version callbacks #358

Conversation

zfields commented Mar 12, 2017 • edited Loading

zfields commented Mar 12, 2017 • edited Loading

soundanalogous commented Mar 13, 2017

soundanalogous commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields commented Mar 13, 2017

soundanalogous Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

zfields Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

soundanalogous Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soundanalogous Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields commented Mar 15, 2017

zfields commented Mar 12, 2017 •

edited

Loading

zfields commented Mar 12, 2017 •

edited

Loading

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017 •

edited

Loading

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017 •

edited

Loading

zfields Mar 13, 2017 •

edited

Loading

zfields Mar 13, 2017 •

edited

Loading

zfields Mar 13, 2017 •

edited

Loading

soundanalogous Mar 13, 2017 •

edited

Loading